Skip to content

Conversation

@kg
Copy link
Member

@kg kg commented Jan 28, 2026

No description provided.

@kg kg added arch-wasm WebAssembly architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jan 28, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

@kg
Copy link
Member Author

kg commented Jan 28, 2026

Not sure what i'm doing wrong here yet.


*************** Starting PHASE Lowering nodeinfo
compEnregLocals() is false, setting doNotEnreg flag for all locals.
Local V00 should not be enregistered because: opts.compFlags & CLFLG_REGVAR is not set

Local V01 should not be enregistered because: opts.compFlags & CLFLG_REGVAR is not set

Local V02 should not be enregistered because: opts.compFlags & CLFLG_REGVAR is not set

Local V03 should not be enregistered because: opts.compFlags & CLFLG_REGVAR is not set
BB02: already in WASM value stack order
lowering store lcl var/field (before):
N001 (???,???) [000002] -----+-----                    t2 =    CNS_INT   int    0
                                                            /--*  t2     int    
N002 (???,???) [000003] DA---+-----                         *  STORE_LCL_VAR struct<Program+S, 16> V03 loc1         


Local V03 should not be enregistered because: written/read in a block op
lowering store lcl var/field (after):
N001 (???,???) [000002] -----+-----                    t2 =    CNS_INT   int    0
               [000012] D----------                   t12 =    LCL_ADDR  byref  V03 loc1         [+0]
                                                            /--*  t12    byref  
                                                            +--*  t2     int    
N002 (???,???) [000003] sA---+-----                         *  STORE_BLK struct<Program+S, 16> (init)


Local V03 should not be enregistered because: written/read in a block op
lowering return node
N001 (???,???) [000007] -----+-----                         *  RETURN    void  
============


Program:copyStruct(ptr) - NYI (Z:\runtime\src\coreclr\jit\lowerwasm.cpp:476 - NYI_WASM: IR not in a stackified form)
Z:\runtime\src\coreclr\jit\lowerwasm.cpp:476
Assertion failed 'NYI_WASM: IR not in a stackified form' in 'Program:copyStruct(ptr)' during 'Lowering nodeinfo' (IL size 17; hash 0xce1e49ee; MinOpts)

{
blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindLoop;
src->SetContained();
// memory.fill
Copy link
Member

@EgorBo EgorBo Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case it's expected to emit a loop of pointer-sized stores (with 0 value) to guarantee atomicity for gc slots (which memset does not guarantee), I am not sure it's a problem today for WASM, but it's probably better to maintain that

Copilot AI review requested due to automatic review settings January 29, 2026 17:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements infrastructure for WebAssembly block store operations in the RyuJIT compiler. It adds support for lowering block copy and initialization operations to use WASM-specific memory.copy and memory.fill instructions.

Changes:

  • Adds LowerBlockStore implementation for WASM target to handle block copy and initialization operations
  • Introduces two new WASM instructions (memory_copy and memory_fill) and their associated instruction format (IF_MEMCPY)
  • Adds emitter support for encoding and displaying the new IF_MEMCPY instruction format

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
src/coreclr/jit/lowerwasm.cpp Implements LowerBlockStore to handle GT_STORE_BLK and GT_INIT_BLK operations, with partial infrastructure for memory.copy and memory.fill
src/coreclr/jit/instrswasm.h Adds memory_copy and memory_fill instruction definitions with their opcodes
src/coreclr/jit/emitfmtswasm.h Defines the IF_MEMCPY instruction format for memory operations with two memory indices
src/coreclr/jit/emitwasm.cpp Implements encoding, size calculation, and display logic for IF_MEMCPY instruction format

// TODO-1stClassStructs: for now we can't work with STORE_BLOCK source in register.
// TODO-WASM: Is this true for wasm as well?
const unsigned srcLclNum = src->AsLclVar()->GetLclNum();
comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::BlockOp));
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using DoNotEnregisterReason::StoreBlkSrc instead of DoNotEnregisterReason::BlockOp for consistency with other architectures (see lowerxarch.cpp:473). StoreBlkSrc is more specific and indicates this is specifically for STORE_BLK sources.

Suggested change
comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::BlockOp));
comp->lvaSetVarDoNotEnregister(srcLclNum DEBUGARG(DoNotEnregisterReason::StoreBlkSrc));

Copilot uses AI. Check for mistakes.
INST(memory_copy, "memory.copy", 0, IF_MEMCPY, 0x10FC)
INST(memory_fill, "memory.fill", 0, IF_ULEB128, 0x11FC)


Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line should be removed for consistency with the rest of the file, which uses single blank lines between sections.

Suggested change

Copilot uses AI. Check for mistakes.
Comment on lines +191 to +194
else
{
// memory.fill
}
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path leaves blkNode->gtBlkOpKind unset (it remains BlkOpKindInvalid). When codegen encounters this node, it will fail because there's no handling for WASM memory.fill instruction yet. Either this should set an appropriate operation kind, or GT_STORE_BLK handling should be added to codegenwasm.cpp before this PR is merged.

Copilot uses AI. Check for mistakes.
Comment on lines +224 to +228
else
{
assert(blkNode->OperIs(GT_STORE_BLK));
// memory.copy
}
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path leaves blkNode->gtBlkOpKind unset (it remains BlkOpKindInvalid). When codegen encounters this node, it will fail because there's no handling for WASM memory.copy instruction yet. Either this should set an appropriate operation kind, or GT_STORE_BLK handling should be added to codegenwasm.cpp before this PR is merged.

Copilot uses AI. Check for mistakes.
IF_DEF(F64, IS_NONE, NONE) // <opcode> <f64 immediate (stored as 64-bit integer constant)>
IF_DEF(MEMARG, IS_NONE, NONE) // <opcode> <memarg> (<align> <offset>)
IF_DEF(LOCAL_DECL, IS_NONE, NONE) // <ULEB128 immediate> <byte>
IF_DEF(MEMCPY, IS_NONE, NONE) // <memory index> <memory index>
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should be <opcode> <memory index> <memory index> to be consistent with other instruction format comments that include the opcode in their description (e.g., ULEB128, SLEB128, MEMARG).

Suggested change
IF_DEF(MEMCPY, IS_NONE, NONE) // <memory index> <memory index>
IF_DEF(MEMCPY, IS_NONE, NONE) // <opcode> <memory index> <memory index>

Copilot uses AI. Check for mistakes.
case IF_MEMCPY:
{
size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this));
size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this));
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing break statement causes fall-through to the default case, which will call unreached() and cause a runtime error. This will prevent the IF_MEMCPY instruction format from working correctly.

Suggested change
size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this));
size += idIsCnsReloc() ? PADDED_RELOC_SIZE : SizeOfULEB128(emitGetInsSC(this));
break;

Copilot uses AI. Check for mistakes.
cnsval_ssize_t imm = emitGetInsSC(id);
printf(" %llu %llu", (uint64_t)imm, (uint64_t)imm);
dispJumpTargetIfAny();
}
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing break statement causes fall-through to the IF_LOCAL_DECL case. This will result in incorrect output when displaying IF_MEMCPY instructions, as it will also print the local declaration information.

Suggested change
}
}
break;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants